Skip to content

Conversation

@gmao-rreichle
Copy link
Collaborator

@gmao-rreichle gmao-rreichle commented Jun 12, 2025

Minor fixes to python package for post-processing ObsFcstAna output.

cc: @amfox37

@gmao-rreichle
Copy link
Collaborator Author

@gmao-qliu, I started a new PR for the ObsFcstAna package because I forgot to ask you about a couple things before merging #87:

  • "domain" was specified in exp_sup1, but there was no check on consistency with "domain" of exp_main. I think that as long as the two experiments are in the same tile space, cross-masking would work even when the domains are different. But I'm not sure if this general functionality would have worked, and it certainly wasn't documented. It's probably simplest to require that the "domain" is the same across all experiments, and to specify "domain" only for exp_main. I took a stab at implementing this in the first commit of the PR. Please double check.
  • Re.
    grid_data, uy, ux = remap_1d_to_2d(tile_data, lat_1d = tc['com_lat'], lon_1d = tc['com_lon'])
    Does this work for a cube-sphere tile space? For the cs tile space, the obs are effectively on an unstructured grid (wherever the com_lat/lon of the tiles happen to be), and the 2d array would probably require huge x and y dimensions (effectively N_tile-by-N_tile), which may crash the node. We haven't had much experience with plotting obs space maps when the model is in cs-space. I think we probably need the true "tile2grid()" functionality, which isn't what remap_1d_to_2d() provides. Perhaps I'm misreading the code. I suspect this needs further work.

@amfox37
Copy link
Contributor

amfox37 commented Jun 15, 2025

I've been using this package quite extensively over the last few days and it seems great for my use cases at least.
The only question I have is whether the Plot_stats_timeseries.py should combine all species into a single mean by default
i.e. OmFm = np.nansum(OmFm*Nobsm)/Nobsm
As this then means the spatial_stats file is a little inconsistent with the temporal_stats file which has output by species.

@gmao-qliu
Copy link
Contributor

I've been using this package quite extensively over the last few days and it seems great for my use cases at least. The only question I have is whether the Plot_stats_timeseries.py should combine all species into a single mean by default i.e. OmFm = np.nansum(OmFm*Nobsm)/Nobsm As this then means the spatial_stats file is a little inconsistent with the temporal_stats file which has output by species.

I can see that one might want to save stats of individual species and only combine all species before plotting. I modified to enable it.

@gmao-qliu
Copy link
Contributor

@gmao-rreichle @amfox37 please review the changes made.

…, Plot_stats_maps.py):

- removed Nmin from calc_temporal_stats_from_sums()
- moved read of stats file to Plot_stats_maps.py
- edited/added comments
- fixed vertical alignment
Copy link
Collaborator Author

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmao-qliu : The main scripts are still rather confusing. I added a few commits and comments but ran out of time to really wrap my mind around the whole thing. In a nutshell, I think we need to clearly separate the tasks accomplished by the various "main" scripts. If we have plot functions, they should focus on plotting and avoid doing things like averaging metrics across species or applying the Nmin threshold. The latter would be better off in the main() portion of the respective scripts. Not sure if all of the comments taken together make sense. We can discuss in person tomorrow

@gmao-rreichle gmao-rreichle changed the title minor fixes for ObsFcstAna postprocessing package revisions of ObsFcstAna postprocessing package Jun 18, 2025
@gmao-qliu
Copy link
Contributor

I've completed the latest round of changes. Please review and test. @gmao-rreichle @amfox37

Copy link
Collaborator Author

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmao-qliu : I made a few more changes (https://github.com/GEOS-ESM/GEOSldas_GridComp/compare/f5fa08c..b4db565?w=1; white-space changes hidden) and added a few comments below.

@gmao-rreichle gmao-rreichle added 0-diff trivial very, very obvious 0-diff change and removed 0-diff labels Jun 24, 2025
@gmao-rreichle gmao-rreichle marked this pull request as ready for review June 25, 2025 20:17
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner June 25, 2025 20:17
@gmao-rreichle gmao-rreichle merged commit c287bc0 into develop Jun 25, 2025
8 checks passed
@gmao-rreichle gmao-rreichle deleted the feature/rreichle/postproc_ObsFcstAna_fixes branch June 25, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-diff trivial very, very obvious 0-diff change post-processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants